Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Return driver object only when valid #93

Merged
merged 3 commits into from
Oct 10, 2024
Merged

Return driver object only when valid #93

merged 3 commits into from
Oct 10, 2024
+4 −2

Conversation

mabraham
Copy link
Contributor

@mabraham mabraham commented Oct 9, 2024

Without this, a driver that failed the sanity check could be returned. This leads to attempting to use it when checking URLs, which led to avoidable failures.

Fixes #92

@mabraham
Return driver object only when valid
a582038 
Without this, a driver that failed the sanity check could be returned. This leads to attempting to use it when checking URLs, which led to avoidable failures.

Fixes urlstechie#92
SuperKogito
SuperKogito previously approved these changes Oct 9, 2024
View reviewed changes
Copy link
Member

@SuperKogito SuperKogito left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm 👍

@vsoch
Copy link
Collaborator

vsoch commented Oct 9, 2024

@mabraham if this fixes the error for you, please bump the version in the changelog and up the version in version.py. I'd also prefer the design pattern of having the driver set back to None if the error is triggered instead of having driver and default_driver (two variables) which I find more confusing.

@SuperKogito
Copy link
Member

I'd also prefer the design pattern of having the driver set back to None if the error is triggered instead of having driver and default_driver (two variables) which I find more confusing.

@vsoch Can you explain why? I personally don't have a preference but I find this implementation cleaner.

@vsoch
Copy link
Collaborator

vsoch commented Oct 9, 2024

@vsoch Can you explain why? I personally don't have a preference but I find this implementation cleaner.

It's simple - as a user developer reading the code I only need to track one instead of two variables.

@mabraham
Copy link
Contributor Author

@mabraham if this fixes the error for you, please bump the version in the changelog and up the version in version.py. I'd also prefer the design pattern of having the driver set back to None if the error is triggered instead of having driver and default_driver (two variables) which I find more confusing.

All done!

@vsoch vsoch merged commit 11dd54a into urlstechie:master Oct 10, 2024
4 checks passed
@vsoch
Copy link
Collaborator

vsoch commented Oct 10, 2024

Thank you @mabraham!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsoch vsoch vsoch approved these changes

@SuperKogito SuperKogito SuperKogito approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Web driver is not installed, but seems to be required
3 participants
@mabraham @vsoch @SuperKogito